Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

2.28: Fix AESNI selection #8209

Merged
merged 6 commits into from
Sep 26, 2023
Merged

2.28: Fix AESNI selection #8209

merged 6 commits into from
Sep 26, 2023

Conversation

lpy4105
Copy link
Contributor

@lpy4105 lpy4105 commented Sep 14, 2023 •
edited by minosgalanakis
Loading

Description

Fix: #8168

This PR does a clean-up of the arch detection and implementation selection macros in include/mbedtls/aesni.h, similar to #7384. This would fix the issue when building with MSVC for ARM/ARM64 target if MBEDTLS_AESNI_C is not disabled.

PR checklist

  • changelogNot required (Cleanup-rework build platoform)
  • backport not required
  • tests not required

@lpy4105 lpy4105 added bug needs-review Every commit must be reviewed by at least two team members, component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review priority-medium Medium priority - this can be reviewed as time permits size-xs Estimated task size: extra small (a few hours at most) labels Sep 14, 2023
@lpy4105 lpy4105 changed the title Fix AESNI selection 2.28: Fix AESNI selection Sep 14, 2023
@lpy4105 lpy4105 mentioned this pull request Sep 14, 2023
Closed
@yanrayw yanrayw self-requested a review September 14, 2023 09:07
@tom-cosgrove-arm tom-cosgrove-arm self-requested a review September 14, 2023 09:36
@tom-cosgrove-arm tom-cosgrove-arm removed the needs-reviewer This PR needs someone to pick it up for review label Sep 14, 2023
@lpy4105 lpy4105 removed the needs-ci Needs to pass CI tests label Sep 15, 2023
yanrayw
yanrayw reviewed Sep 15, 2023
View reviewed changes
Copy link

@yanrayw yanrayw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, only two minor issues.

include/mbedtls/aesni.h Show resolved Hide resolved
include/mbedtls/padlock.h Outdated Show resolved Hide resolved
@lpy4105
Copy link
Contributor Author

lpy4105 commented Sep 21, 2023

Please correct me if I was wrong, I think building ARM/ARM64 target using MSVC is not officially support, so here are two questions:

  • I think this is an 'internal' change, but do we need a bugfix changelog entry?
  • We don't need to provide relative tests on our CI, is that correct?

@lpy4105 lpy4105 mentioned this pull request Sep 21, 2023
Merged
3 tasks
@gilles-peskine-arm gilles-peskine-arm added priority-very-high Highest priority - prioritise this over other review work and removed priority-medium Medium priority - this can be reviewed as time permits labels Sep 21, 2023
@tom-cosgrove-arm
Copy link
Contributor

tom-cosgrove-arm commented Sep 25, 2023 •
edited
Loading

Just to confirm, (cross) building on X86_64 for ARM64 with default config, on 2.28 I get #error: This header is specific to X86 and X64 targets - that is fixed by this PR.

There is no such issue on development

@lpy4105
Fix comment
8068b08 
Signed-off-by: Pengyu Lv <[email protected]>
@lpy4105 lpy4105 requested a review from yanrayw September 26, 2023 01:47
@lpy4105 lpy4105 linked an issue Sep 26, 2023 that may be closed by this pull request
Compile error for AES-NI with MSVC on ARM32 (UWP), AES-NI intrinsics are only on x86 and x64 #8168
Closed
yanrayw
yanrayw approved these changes Sep 26, 2023
View reviewed changes
Copy link

@yanrayw yanrayw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tom-cosgrove-arm
Copy link
Contributor

tom-cosgrove-arm commented Sep 26, 2023 •
edited
Loading

I've also confirmed that we get NI compiled in with intrinsics (and Padlock support, if enabled) when targeting (Linux) 32-bit (with -maes etc), NI compiled in with assembler when targeting Linux/x86_64, and NI via intrinsics when targeting Linux/x86_64 without assembler support (again, needing -maes etc) or Windows/amd64.

Note that if MBEDTLS_HAVE_ASM is turned off, and the user wants AES-NI, but doesn't include the full set of necessary command-line flags (e.g. -mpclmul -msse2 -maes), then the library will silently build without AES-NI

tom-cosgrove-arm
tom-cosgrove-arm approved these changes Sep 26, 2023
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tom-cosgrove-arm tom-cosgrove-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Sep 26, 2023
@daverodgman daverodgman added this pull request to the merge queue Sep 26, 2023
Merged via the queue into Mbed-TLS:mbedtls-2.28 with commit 7a8ec0f Sep 26, 2023
1 check passed
@tom-cosgrove-arm
Copy link
Contributor

Should #8168 not have been closed when this was merged?

@gilles-peskine-arm
Copy link
Contributor

Should #8168 not have been closed when this was merged?

No, that only happens when a pull request is merged into the directory's default branch, which is development. LTS-only issues need to be closed manually.

@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Sep 29, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tom-cosgrove-arm tom-cosgrove-arm tom-cosgrove-arm approved these changes

@yanrayw yanrayw yanrayw approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports bug component-crypto Crypto primitives and low-level interfaces priority-very-high Highest priority - prioritise this over other review work size-xs Estimated task size: extra small (a few hours at most)
Projects
Roadmap Board for Mbed TLS
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Compile error for AES-NI with MSVC on ARM32 (UWP), AES-NI intrinsics are only on x86 and x64
5 participants
@lpy4105 @tom-cosgrove-arm @gilles-peskine-arm @yanrayw @daverodgman